chore: perf optimisation for js action creation phase 1 #37391
Conversation
WalkthroughThe changes in this pull request primarily involve modifications to the Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
Failed server tests
|
1 similar comment
Failed server tests
|
github-actions
bot
added
Integrations Pod General
Failed server tests
|
… to retain the previous logic we had
Failed server tests
|
| @@ -335,11 +339,23 @@ public Mono<ActionCollectionDTO> updateUnpublishedActionCollection( | |||
| actionDTO.setPluginType(actionCollectionDTO.getPluginType()); | |||
| actionDTO.setPluginId(actionCollectionDTO.getPluginId()); | |||
| actionDTO.setBranchName(branchedActionCollection.getBranchName()); | |||
|
|
|||
| boolean isDuplicateActionName = existingActions.stream() | |||
There was a problem hiding this comment.
Why are we checking only against existing names and not against all incoming actions?
There was a problem hiding this comment.
makes sense, ill check for all
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceTest.java (1)
870-928: LGTM! Consider extracting common test setup.The test effectively validates handling of multiple actions with duplicate names. Consider extracting the common setup code (action collection creation, mock setup) into a helper method to improve maintainability.
+ private ActionCollectionDTO createTestActionCollection(String name, String actionName) { + ActionCollectionDTO actionCollectionDTO = new ActionCollectionDTO(); + actionCollectionDTO.setName(name); + actionCollectionDTO.setPageId(testPage.getId()); + actionCollectionDTO.setApplicationId(testApp.getId()); + actionCollectionDTO.setWorkspaceId(workspaceId); + actionCollectionDTO.setPluginId(datasource.getPluginId()); + actionCollectionDTO.setVariables(List.of(new JSValue("test", "String", "test", true))); + actionCollectionDTO.setBody("collectionBody"); + actionCollectionDTO.setPluginType(PluginType.JS); + + ActionDTO action = new ActionDTO(); + action.setName(actionName); + action.setActionConfiguration(new ActionConfiguration()); + action.getActionConfiguration().setBody("initial body"); + action.getActionConfiguration().setIsValid(false); + actionCollectionDTO.setActions(List.of(action)); + + return actionCollectionDTO; + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java(2 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceTest.java(3 hunks)
🔇 Additional comments (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java (1)
439-446: Verify client-side validation for JS action names
While bypassing server-side validation for JS actions improves performance, we need to ensure that client-side validation is robust.
Consider adding debug logs before the conditional check:
+ log.debug("Validating action name. Action type: {}, Name: {}", isJsAction ? "JS" : "Non-JS", name);
if (!isJsAction) {
return refactoringService
.isNameAllowed(page.getId(), contextType, layout.getId(), name)
.name(IS_NAME_ALLOWED)
.tap(Micrometer.observation(observationRegistry));
}
+ log.debug("Skipping name validation for JS action: {}", name);
return Mono.just(true);app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java (2)
318-325: LGTM! Efficient approach for duplicate name validation.
The stream-based implementation efficiently validates unique action names in memory before any DB operations.
346-358: Consider adding transaction boundary for concurrent action creation.
While the duplicate name check works for single requests, there's a potential race condition when multiple requests attempt to create actions with the same name simultaneously. Consider wrapping the name check and action creation in a transaction.
Let's verify if there are any existing transaction boundaries:
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceTest.java (1)
810-868: LGTM! Well-structured test for duplicate action name validation.
The test effectively validates the error handling when attempting to create a new action with the same name as an existing one.
Description
During analysis of action creation flow metrics, we observed that RefactoringService.isNameAllowed is taking 80-90% of the total JS object action time. This PR optimises this part in a way that for any jsobject action, instead of fetching all actions from DB and comparing it
to see if current action name is allowed, we simply do that check in memory where for current action collection, if any action names are being duplicated, we throw the error.
We could make this change easily because recently we merged a PR which removes the actions with duplicate name from client payload whenever Js object update API is called, with this change, we can guarantee that for any JS object update call, all actions inside it will always have unique names. This PR makes the similar check on backend where if any action has duplicate name within collection, we throw an error and don't store that action in the DB.
We may need to consider following test case in both before and after implementation of this approach. This can be covered during PR testing:
What happens if the client sends multiple requests to add a new function in an existing collection. That is, as a result of the debounce logic, if the server receives 2 consecutive requests with a populated collection but without actionId associated to either request.
Relevant thread: https://theappsmith.slack.com/archives/C040LHZN03V/p1731571364933089
Fixes #37365
or
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11852609812
Commit: d5c75ed
Cypress dashboard.
Tags: @tag.All
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Regression/ClientSide/Binding/Api_withPageload_Input_spec.js
- cypress/e2e/Regression/ClientSide/IDE/Focus_retentions_inputs_spec.js
- cypress/e2e/Regression/ServerSide/GenerateCRUD/S3_Spec.js
List of identified flaky tests.Fri, 15 Nov 2024 09:23:23 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Tests